Skip to content

TST: fix errant tight_layout test #15671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 13, 2017

Conversation

ResidentMario
Copy link
Contributor

@ResidentMario ResidentMario commented Mar 13, 2017

closes #9351

@jorisvandenbossche
Copy link
Member

Why is it needed to remove this test?

@ResidentMario
Copy link
Contributor Author

The test itself is incorrect because the 2 should be a 3 (not my best moment, this was). Additionally, this is patched in matplotlib 2.0.1, not matplotlib 2.0.0 as indicated here. So to keep the test around, we would need to modify when it skips.

More substantially, this bug was actually a bug in matplotlib, which I fixed and added a regression test for in their test suite. pandas doesn't really need to duplicate it.

@jorisvandenbossche
Copy link
Member

Ah, the reason that it is 2 instead of 3 is the reason it now just passes, while the test should actually fail because it needs >= 2.0.1 instead of >= 2.0.0 ?

I think testing this in pandas is fine, as we should keep tight_layout working with pandas plots (we may change implementation details in the hist method that could cause problems with tight_layout)

@ResidentMario
Copy link
Contributor Author

Yes, exactly.

Hmm. OK, can do.

@codecov-io
Copy link

codecov-io commented Mar 13, 2017

Codecov Report

Merging #15671 into master will decrease coverage by 0.02%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master   #15671      +/-   ##
==========================================
- Coverage   91.03%   91.01%   -0.03%     
==========================================
  Files         143      143              
  Lines       49380    49386       +6     
==========================================
- Hits        44953    44947       -6     
- Misses       4427     4439      +12
Impacted Files Coverage Δ
pandas/tools/plotting.py 71.77% <66.66%> (-0.02%)
pandas/io/gbq.py 25% <0%> (-58.34%)
pandas/tools/merge.py 93.61% <0%> (-0.34%)
pandas/core/frame.py 97.87% <0%> (-0.1%)
pandas/core/common.py 91.3% <0%> (+0.33%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32df1e6...99cd255. Read the comment docs.

@ResidentMario ResidentMario changed the title TST: remove errant test TST: fix errant tight_layout test Mar 13, 2017
@ResidentMario
Copy link
Contributor Author

@jreback @jorisvandenbossche Does this look good? Tests pass.

@ResidentMario
Copy link
Contributor Author

This would close #9351.

@jreback jreback added the Visualization plotting label Mar 13, 2017
@jreback jreback added this to the 0.20.0 milestone Mar 13, 2017
@jreback
Copy link
Contributor

jreback commented Mar 13, 2017

lgtm. @jorisvandenbossche

@jorisvandenbossche jorisvandenbossche merged commit 998c801 into pandas-dev:master Mar 13, 2017
@jorisvandenbossche
Copy link
Member

Thanks!

@ResidentMario ResidentMario deleted the patch-1 branch March 14, 2017 18:25
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request Mar 21, 2017
mattip pushed a commit to mattip/pandas that referenced this pull request Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataFrame.hist() does not get along with matplotlib.pyplot.tight_layout()
4 participants